Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise mapped tuple type instantiation logic #57031

Merged
merged 7 commits into from
Jan 19, 2024
Merged

Revise mapped tuple type instantiation logic #57031

merged 7 commits into from
Jan 19, 2024

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Jan 11, 2024

This PR revises and simplifies our mapped tuple type instantiation logic. Previously, when a mapped type was applied to a variadic tuple type, we'd transform the operation into a sequence of operations on manufactured singleton tuple types. This would have surprising effects when observing property key types, as reported in #48856. For example:

type Keys<T> = { [K in keyof T]: K };
type Foo<T extends unknown[]> = Keys<[string, string, ...T, string]>;  // ["0", "0", ...Keys<T>, "0"]

With this PR, the property keys are more consistent:

type Keys<T> = { [K in keyof T]: K };
type Foo<T extends unknown[]> = Keys<[string, string, ...T, string]>;  // ["0", "1", ...Keys<T>, number]

The PR also fixes issues related to instantiation of tuples with required elements following a rest element, as reported in #56888, and it fixes the error reporting issue discussed here.

Finally, the PR reverts #53522. The fix in that PR is incorrect--it effectively leaks a type parameter. With this PR, the repro from #53458 now causes an excessive instantiation depth error, which should have been the behavior in the first place.

Fixes #48856.
Fixes #56888.

@ahejlsberg
Copy link
Member Author

@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 11, 2024

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 2980eae. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 11, 2024

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 2980eae. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 11, 2024

Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 2980eae. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 11, 2024

Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at 2980eae. You can monitor the build here.

Update: The results are in!

// apply the mapped type itself to the variadic element type. For other elements in the variable part of the
// tuple, we surround the element type with an array type and apply the mapped type to that. This ensures
// that we get sequential property key types ("0", "1", "2", etc.) for the fixed part of the tuple, and
// property key type number for the remaining elements.
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you keep something similar to the original example in place? Or is it too different now for some reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and as you note the example with the keys, maybe include the example from your PR description as well?

type Keys<T> = { [K in keyof T]: K };
type Foo<T extends unknown[]> = Keys<[string, string, ...T, string]>;  // ["0", "1", ...Keys<T>, number]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is, the original transformation (as witnessed by that example) was inconsistent with regards to property keys, so not sure there's any value in keeping it.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking of trying to update the example, not keep it.

I guess the issue you might've run into is that we don't have great notation to describe this. The closest thing being something like:

We transform

M<[A, B?, ...T, ...C[]]

into

[M<{ 0: A }>[0], M<{ 1: B }>[1]?, ...M<T>, ...M<C[]>]

which is not too terrible.

But I think having an example still has value, so if we have to go concrete, the Keys example isn't bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the Keys example. In general, we now process the fixed part of the tuple just like any other object. It's really just the elements in the variable part that get special processing.

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,502k (± 0.01%) 295,491k (± 0.01%) ~ 295,457k 295,513k p=0.471 n=6
Parse Time 2.65s (± 0.15%) 2.65s (± 0.31%) ~ 2.65s 2.67s p=0.218 n=6
Bind Time 0.82s (± 0.00%) 0.82s (± 0.99%) ~ 0.82s 0.84s p=0.405 n=6
Check Time 8.13s (± 0.39%) 8.14s (± 0.23%) ~ 8.12s 8.16s p=0.935 n=6
Emit Time 7.10s (± 0.35%) 7.10s (± 0.23%) ~ 7.07s 7.11s p=0.668 n=6
Total Time 18.70s (± 0.26%) 18.71s (± 0.13%) ~ 18.69s 18.75s p=0.935 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,494k (± 1.22%) 192,521k (± 1.21%) ~ 191,509k 197,275k p=0.873 n=6
Parse Time 1.35s (± 1.46%) 1.36s (± 0.93%) ~ 1.35s 1.38s p=0.210 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.34s (± 0.48%) 9.36s (± 0.23%) ~ 9.32s 9.38s p=0.419 n=6
Emit Time 2.63s (± 0.39%) 2.63s (± 0.40%) ~ 2.61s 2.64s p=0.203 n=6
Total Time 14.04s (± 0.35%) 14.07s (± 0.18%) ~ 14.02s 14.09s p=0.327 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,391k (± 0.00%) 347,391k (± 0.00%) ~ 347,368k 347,414k p=0.936 n=6
Parse Time 2.46s (± 0.34%) 2.46s (± 0.21%) ~ 2.45s 2.46s p=0.070 n=6
Bind Time 0.92s (± 0.59%) 0.92s (± 0.59%) ~ 0.92s 0.93s p=1.000 n=6
Check Time 6.86s (± 0.34%) 6.86s (± 0.36%) ~ 6.84s 6.90s p=0.934 n=6
Emit Time 4.06s (± 0.20%) 4.04s (± 0.48%) ~ 4.02s 4.07s p=0.085 n=6
Total Time 14.30s (± 0.14%) 14.28s (± 0.25%) ~ 14.24s 14.34s p=0.227 n=6
TFS - node (v18.15.0, x64)
Memory used 302,777k (± 0.00%) 302,784k (± 0.01%) ~ 302,769k 302,813k p=0.810 n=6
Parse Time 2.01s (± 1.03%) 2.00s (± 0.80%) ~ 1.98s 2.02s p=0.685 n=6
Bind Time 1.00s (± 0.75%) 1.00s (± 1.21%) ~ 0.99s 1.02s p=0.867 n=6
Check Time 6.30s (± 0.38%) 6.31s (± 0.33%) ~ 6.28s 6.34s p=0.868 n=6
Emit Time 3.59s (± 0.29%) 3.58s (± 0.48%) ~ 3.56s 3.61s p=0.285 n=6
Total Time 12.90s (± 0.29%) 12.89s (± 0.10%) ~ 12.88s 12.91s p=1.000 n=6
material-ui - node (v18.15.0, x64)
Memory used 508,278k (± 0.00%) 508,289k (± 0.01%) ~ 508,269k 508,335k p=0.375 n=6
Parse Time 2.59s (± 0.66%) 2.59s (± 0.57%) ~ 2.58s 2.62s p=0.611 n=6
Bind Time 0.99s (± 0.99%) 1.00s (± 1.05%) ~ 0.98s 1.01s p=0.343 n=6
Check Time 17.13s (± 0.45%) 17.17s (± 0.29%) ~ 17.10s 17.23s p=0.572 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.71s (± 0.42%) 20.75s (± 0.24%) ~ 20.68s 20.80s p=0.572 n=6
xstate - node (v18.15.0, x64)
Memory used 512,990k (± 0.01%) 512,969k (± 0.01%) ~ 512,850k 513,053k p=0.575 n=6
Parse Time 3.27s (± 0.25%) 3.28s (± 0.17%) ~ 3.27s 3.28s p=0.859 n=6
Bind Time 1.54s (± 0.34%) 1.53s (± 0.34%) ~ 1.53s 1.54s p=0.311 n=6
Check Time 2.83s (± 0.37%) 2.82s (± 0.76%) ~ 2.79s 2.85s p=0.370 n=6
Emit Time 0.07s (± 5.69%) 0.07s (± 0.00%) ~ 0.07s 0.07s p=0.405 n=6
Total Time 7.71s (± 0.24%) 7.71s (± 0.29%) ~ 7.67s 7.73s p=0.871 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/57031/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

const flags = elementFlags[i];
return i < fixedLength ? instantiateMappedTypeTemplate(mappedType, getStringLiteralType("" + i), !!(flags & ElementFlags.Optional), fixedMapper) :
flags & ElementFlags.Variadic ? instantiateType(mappedType, prependTypeMapping(typeVariable, type, mapper)) :
getElementTypeOfArrayType(instantiateType(mappedType, prependTypeMapping(typeVariable, createArrayType(type), mapper))) || unknownType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getElementTypeOfArrayType(instantiateType(mappedType, prependTypeMapping(typeVariable, createArrayType(type), mapper))) || unknownType;
getElementTypeOfArrayType(instantiateType(mappedType, prependTypeMapping(typeVariable, createArrayType(type), mapper))) ?? unknownType;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will fix.

type KA = K<[string, string, boolean]>; // ["0", "1", "2"]
type KB = K<[string, string, ...string[], string]>; // ["0", "1", ...number[], number]
type KC = K<[...string[]]>; // number[]
type KD = K<string[]>; // number[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not allowed to have non-generic variable elements follow a non-generic rest, but it would be good to make sure the checker tolerates states like that

Suggested change
type KD = K<string[]>; // number[]
type KD = K<string[]>; // number[]
type KE = K<[string, ...boolean[], number?]>;
type KE = K<[string, ...boolean[], ...number[]]>;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you bring this up here... this is killing me 😅

type A = [number, ...boolean[], ...string[]] // error
type B = [number, ...Array<boolean>, ...Array<string>] // ok

Shouldn't those really behave the same way? IIRC I have a PR laying around somewhere that makes this error go away (by matching the behavior of the latter)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the PR already open?

Not that it's related, but you inspired me to file something else I've put off mentioning for a while: #57034

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably not the intention that we want to allow that code to be honest. But I'll let Anders weigh in.

Copy link
Contributor

@Andarist Andarist Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the PR already open?

sure thing :) #55446

and even more surprising to the user is that you can make the error go away when moving string[] to a type alias:

type Alias = string[]
type Ok2 = [...number[], ...Alias]

So this whole rule prevents only a very narrow set of scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRosenwasser Those extra examples wouldn't really check anything related to this PR since we normalize the tuple type before the instantiation logic even sees it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andarist I still think the error makes sense, but we should fix the error reporting logic to be consistent between the notations. Permitting [...string[], ...number[]] with no errors will just make users think we can actually check for a sequence of strings followed by a sequence of numbers, but that's not the case.

Copy link
Contributor

@Andarist Andarist Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume that people might already be relying on the fact that spreading like this might "normalize" the shape, so I thought that it is off the table as it would be considered a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error reporting fixed with the latest commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error reporting fixed with the latest commit.

Thank you ❤️ I like that this will be more consistent now.

I found the old behavior (without the error) moderately useful. It's cool that it will still be possible - just somewhat unfortunate that to benefit from it we'll have to use an extra type wrapper (TS playground):

type A = [number, string?];
type B = [...boolean[], boolean];
// ok in 5.3, errors with the change
type C = [bigint, ...bigint[], ...B, ...A]; // [bigint, ...(string | number | bigint | boolean | undefined)[]]

type Indirect<A1 extends unknown[], A2 extends unknown[], A3 extends unknown[], A4 extends unknown[]> = [...A1, ...A2, ...A3, ...A4]
// ok in 5.3, ok with the change
type IndirectResult = Indirect<[bigint], bigint[], B, A> // [bigint, ...(string | number | bigint | boolean | undefined)[]]

@DanielRosenwasser
Copy link
Member

Looks good, left some comments that should be quick to address.

@typescript-bot
Copy link
Collaborator

Hey @ahejlsberg, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

if (isTupleType(t)) {
return instantiateMappedTupleType(t, type, prependTypeMapping(typeVariable, t, mapper));
return instantiateMappedTupleType(t, type, typeVariable, mapper);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An offtopic question if I may. I noticed (on several occasions already) that instantiating array/tuple mapped types eagerly~ leads to inconsistent behaviors with the object variants. The timing of instantiation is quite different and thus the availability of certain information tends to be different between them.

What are your thoughts on making this more consistent? How feasible that would even be? When it comes to objects each property stays deferred for as much as possible and I imagine that this would require deferring this instantiation per tuple element.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the advantage of eager instantiation is it is simpler to implement and it allows sharing of identical instantiations. The specific reason we eagerly instantiate mapped array and tuple types is that elsewhere in the compiler we detect arrays and tuples by looking for instantiations of Array<T> and the corresponding synthetic tuple classes. We'd have to extend those cases to also handle mapped types applied to arrays and tuples.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the answer. I might want to attempt unifying this at some point - it's good to know that you don't have strong objections 😉 Or at least I read that from your comment ;p I understand that this might introduce some new complexity but I think it might be inevitable (as long as the goal is to have consistent behavior). When trying to do this, I will gather the test cases backing up that it's a desired change.

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the top-repos suite comparing main and refs/pull/57031/merge:

Something interesting changed - please have a look.

Details

chakra-ui/chakra-ui

4 of 28 projects failed to build with the old tsc and were ignored

packages/components/tsconfig.build.json

  • error TS5056: Cannot write file '/mnt/ts_downloads/chakra-ui/packages/components/dist/types/menu/menu.stories.d.ts' because it would be overwritten by multiple input files.
    • Project Scope

type V15 = [...string[], ...number[]]; // Error
type V16 = [...string[], ...Array<number>]; // Error
type V17 = [...Array<string>, ...number[]]; // Error
type V18 = [...Array<string>, ...Array<number>]; // Error
Copy link
Contributor

@Andarist Andarist Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose some extra test cases with type aliases:

Suggested change
type V18 = [...Array<string>, ...Array<number>]; // Error
type V18 = [...Array<string>, ...Array<number>]; // Error
type Alias19 = number[];
type V19 = [...string[], ...Alias19]; // Error
type Alias20 = Array<number>;
type V20 = [...string[], ...Alias20]; // Error

@Andarist
Copy link
Contributor

It might be worth rechecking if the recently merged in #55774 doesn't throw any wrinkles into this.

@jakebailey
Copy link
Member

I checked, and this PR still passes with main merged in (unless you were thinking of something other effect).

@Andarist
Copy link
Contributor

Andarist commented Jan 18, 2024

Unfortunately, #55774 got reverted because it weirdly clashed with another PR that was merged recently ( #56727 ). However, I didn't think that a merge of those would fail any existing test cases but rather that maybe some new test cases should be written in light of this PR. I just had a hunch of sorts that maybe there is something to be re-checked but I didn't have the energy myself at the time to think about the actual test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
6 participants